Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a CI that fails if line length exceeds 80 #1231

Closed
wants to merge 5 commits into from
Closed

Conversation

rnjtranjan
Copy link
Collaborator

Fix #1218

@harendra-kumar
Copy link
Member

hlint is fixed in my refactoring commit.

@@ -390,7 +390,8 @@ jobs:
command: |
bash -c "$PACKCHECK $BUILD" || exit 1
count=$(find . -name "*.hs" -exec grep -H '\ $' {} \; | tee /dev/tty | wc -l)
exit $count
count2=$(awk '{ if (length($0) > 80) {print "Line length exceeds 80 : " length($0); print $0; print FILENAME; } }' *.hs | tee /dev/tty | wc -l)
exit $((count+count2))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use separate failures for trailing spaces and for line length. How are we going to know which one failed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only processing *.hs files in the current directory. There are no such files in the repo root. So it would always succeed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can we do the line-length processing on git diff output so that we do not have to worry about fixing the existing long lines.

Copy link
Member

@harendra-kumar harendra-kumar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase, to resolve conflicts.

if [ $count > 0 ]
then exit 1
fi
git diff > tmp_len
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work. The output will always be empty as there is nothing changed in the checked out tree. You need to do git diff between the current branch HEAD and the target branch in which the PR is to be merged. The current branch and target branch info should be available in some env vars.

Also you could just directly pipe the output of diff to the command below rather than putting it in a file first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In github, ${{ github.event.pull_request.head.sha }} is the pull request base commit.

git diff ${{ github.event.pull_request.head.sha }} HEAD 

will work for all pull requests as expected.

In circleci, I am unable to immediately find the env var that has the branch or the commit being compared to.
Some interesting env vars are,

  • CIRCLE_BRANCH: Current branch name
  • CIRCLE_SHA1: Current checkout latest hash

A trivial way if the merge branch is always origin/master is:

git diff origin/master HEAD 

You can probably use the github CI.

git diff ${{ github.event.pull_request.head.sha }} HEAD

should work as expected.

dit diff might produce lines that might be over 80 that we have no control of.
These might be the git info messages, separators, etc.
We need to filter only lines that start with a +.
We can probably solve the whole task with sed.

then exit 1
fi
git diff > tmp_len
count2=$(awk '{ if (length($0) > 80) {print "Line length exceeds 80 : " length($0); print $0; } }' tmp_len | tee /dev/tty | wc -l)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The git diff output has deleted lines, changed lines and other info. We need to grep only the lines with a leading + (grep '^+') and filter out the lines with a leading +++(grep -v^+++`) from that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes!

@rnjtranjan
Copy link
Collaborator Author

ner PR opened: #1557

@rnjtranjan rnjtranjan closed this Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a CI that fails if line length exceeds 79
3 participants